Skip to content

feat(core,isthmus): add DynamicParameter expression support#752

Open
bvolpato wants to merge 1 commit intosubstrait-io:mainfrom
bvolpato:bv/dynamic-parameter-support
Open

feat(core,isthmus): add DynamicParameter expression support#752
bvolpato wants to merge 1 commit intosubstrait-io:mainfrom
bvolpato:bv/dynamic-parameter-support

Conversation

@bvolpato
Copy link
Copy Markdown
Member

@bvolpato bvolpato commented Mar 16, 2026

Summary

Add full support for Substrait DynamicParameter expressions, enabling parameterized placeholders (analogous to JDBC ? bind parameters) in plan bodies instead of embedded literals. This maps bidirectionally to Calcite's RexDynamicParam.

DynamicParameter enables plan reuse — the same plan structure can be compiled and cached once, then executed with different parameter values without re-planning. This is essential for prepared statement workflows and for exchanging plans between engines without leaking literal values.

Changes

Core POJO + Proto layer:

  • Expression.DynamicParameter — immutable POJO with type and parameterReference fields
  • ExpressionVisitor / AbstractExpressionVisitor — new visit(DynamicParameter) method
  • ExpressionProtoConverter — POJO→Proto conversion
  • ProtoExpressionConverter — Proto→POJO conversion (DYNAMIC_PARAMETER case)
  • ExpressionCopyOnWriteVisitor — leaf-node handling (returns Optional.empty())

Calcite integration (isthmus):

  • RexExpressionConverter.visitDynamicParam() — replaces UnsupportedOperationException with actual Calcite→Substrait conversion
  • ExpressionRexConverter.visit(DynamicParameter) — Substrait→Calcite conversion

Debug support:

  • ExpressionStringify — added DynamicParameter stringification

Testing

20 tests total, all passing:

  • 8 core proto roundtrip tests (DynamicParameterRoundtripTest in core) — POJO↔Proto↔POJO for I64, nullable STRING, FP64, I32, DATE, BOOLEAN, DECIMAL, TIMESTAMP
  • 5 isthmus unit tests (DynamicParameterTest) — direct Calcite↔Substrait conversions and project roundtrips
  • 7 isthmus end-to-end roundtrip tests (DynamicParameterRoundtripTest in isthmus) — filter/projection/multi-type POJO roundtrips, Calcite-originated RexDynamicParam roundtrips, and full Substrait→Calcite→Substrait bidirectional roundtrips

All existing tests in core and isthmus continue to pass with zero regressions. PMD and Spotless checks are clean.

This was referenced Mar 16, 2026
Merged
Closed
Closed
@nielspardon
Copy link
Copy Markdown
Member

nielspardon commented Mar 18, 2026

Do you plan to also work on implementing the DynamicParameterBinding handling?

bvolpato added a commit to substrait-io/substrait-go that referenced this pull request Mar 22, 2026
@bvolpato
feat: add DynamicParameter expression and plan bindings (#215)
430e842 
## Summary

Add full support for `DynamicParameter` expressions and plan-level
`DynamicParameterBinding`, enabling parameterized queries in
substrait-go.

This mirrors the functionality recently added in
[substrait-java#752](substrait-io/substrait-java#752),
ensuring Go consumers can create and read plans containing dynamic
parameters.

## Changes

### Expression (`expr/expression.go`)
- New `DynamicParameter` struct implementing the full `Expression`
interface (`String`, `ToProto`, `Equals`, `Visit`, `IsScalar`,
`GetType`, `ToProtoFuncArg`)
- Proto deserialization in `ExprFromProto()` with nil safety check

### Builder (`expr/builder.go`)
- `ExprBuilder.DynamicParam(outputType, paramRef)` method with
`dynamicParamBuilder`
- Implements `BuildExpr()` and `BuildFuncArg()` for use as function
arguments

### Plan bindings (`plan/common.go`, `plan/plan.go`, `plan/builders.go`)
- `DynamicParameterBinding` struct mapping parameter anchors to literal
values
- `Plan.ParameterBindings()` accessor
- `FromProto()` / `ToProto()` serialization for parameter bindings
- `Builder.PlanWithBindings()` method on the `Builder` interface

## Testing

17 new tests across two test files:

- `expr/dynamic_parameter_test.go` — 11 tests covering basic
construction, nullability, equality, visit, proto roundtrip (5 types),
nil proto error, builder (3 cases), builder as function argument, and
multiple parameters
- `plan/dynamic_parameter_test.go` — 6 tests covering filter plan with
bindings, project plan, multiple bindings, no bindings, JSON parsing
roundtrip, and end-to-end builder usage

All existing tests continue to pass with zero regressions.
@bvolpato-dd bvolpato-dd force-pushed the bv/dynamic-parameter-support branch 2 times, most recently from a1807e4 to 96bcc42 Compare March 24, 2026 02:08
@bvolpato
Copy link
Copy Markdown
Member Author

bvolpato commented Mar 24, 2026

Thanks @nielspardon! Great question.

I've rebased this PR on the latest main (now includes lambda support and v0.86.1 changes) — conflicts are resolved and all tests pass.

Regarding DynamicParameterBinding — I intentionally scoped this PR to the expression-level DynamicParameter only (the POJO, proto conversion, Calcite↔Substrait mapping, and visitor wiring). The plan-level parameter_bindings field is a separate concern that lives on the Plan message and maps parameter anchors to literal values at execution time.

I think it makes sense to handle DynamicParameterBinding in a follow-up PR for a few reasons:

  1. Separation of concerns: This PR focuses on the expression representation (DynamicParameter in plan bodies). The bindings are a plan-level concept that affects Plan.java, PlanProtoConverter, and ProtoPlanConverter — a different layer.
  2. This PR is already self-contained and useful: Engines can produce/consume plans with DynamicParameter placeholders today, even without bindings. The bindings are optional per the spec ("An optional list of bindings...").
  3. Validation complexity: Proper binding support should include type validation (binding literal type must match the DynamicParameter type in the plan) — similar to what we did in substrait-go with ValidateParameterBindings.

I'll open a follow-up issue to track DynamicParameterBinding handling on the Plan POJO. Happy to discuss if you'd prefer them combined though!

@substrait-io substrait-io deleted a comment from bvolpato-dd Mar 24, 2026
@bvolpato bvolpato mentioned this pull request Mar 24, 2026
Open
@nielspardon
Copy link
Copy Markdown
Member

nielspardon commented Mar 24, 2026

Regarding DynamicParameterBinding — I intentionally scoped this PR to the expression-level DynamicParameter only (the POJO, proto conversion, Calcite↔Substrait mapping, and visitor wiring). The plan-level parameter_bindings field is a separate concern that lives on the Plan message and maps parameter anchors to literal values at execution time.

I wasn't suggesting that it should be included in this PR. I was simply asking whether you were planning to also look into DynamicParameterBinding. A simple yes or no would have been sufficient for me as an answer.

bvolpato
bvolpato commented Apr 14, 2026
View reviewed changes
Copy link
Copy Markdown
Member Author

@bvolpato bvolpato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👋 Checking in on this PR. @nielspardon left some helpful review feedback on March 18 & 24 about test consistency - specifically that some test cases only assert the type while others only assert the parameter reference, and suggested checking both consistently.

This PR has been open for a month - are you planning to address the review feedback? If you're waiting on further review from maintainers, a gentle ping might help move it forward.

@bvolpato
Copy link
Copy Markdown
Member Author

bvolpato commented Apr 14, 2026

(disregard the comment above, I was testing some automation!)

@bvolpato
feat(core,isthmus): add DynamicParameter expression support
1416c34 
Implement full support for Substrait DynamicParameter expressions,
enabling parameterized placeholders in plan bodies instead of
embedded literals. This maps bidirectionally to Calcite's
RexDynamicParam (JDBC ? bind parameters).

Changes:
- Add Expression.DynamicParameter POJO with type and parameterReference
- Wire visitor pattern across all expression visitors
- Add proto conversion (POJO<->Proto) in both directions
- Add Calcite conversion (RexDynamicParam<->DynamicParameter)
- Replace UnsupportedOperationException in visitDynamicParam
- Add debug stringification in ExpressionStringify
- Add 20 tests covering proto roundtrips, Calcite conversions,
  and full end-to-end roundtrips
@bvolpato-dd bvolpato-dd force-pushed the bv/dynamic-parameter-support branch from 96bcc42 to 1416c34 Compare April 26, 2026 05:14
@bvolpato bvolpato requested a review from nielspardon April 26, 2026 05:54
@bvolpato
Copy link
Copy Markdown
Member Author

bvolpato commented May 5, 2026

[review-prs-verification]
Head: 1416c34
Ready to merge: yes

Files reviewed.

  • core/src/main/java/io/substrait/expression/AbstractExpressionVisitor.java
  • core/src/main/java/io/substrait/expression/Expression.java
  • core/src/main/java/io/substrait/expression/ExpressionVisitor.java
  • core/src/main/java/io/substrait/expression/proto/ExpressionProtoConverter.java
  • core/src/main/java/io/substrait/expression/proto/ProtoExpressionConverter.java
  • core/src/main/java/io/substrait/relation/ExpressionCopyOnWriteVisitor.java
  • core/src/test/java/io/substrait/type/proto/DynamicParameterRoundtripTest.java
  • examples/substrait-spark/src/main/java/io/substrait/examples/util/ExpressionStringify.java
  • isthmus/src/main/java/io/substrait/isthmus/expression/ExpressionRexConverter.java
  • isthmus/src/main/java/io/substrait/isthmus/expression/RexExpressionConverter.java
  • isthmus/src/test/java/io/substrait/isthmus/DynamicParameterRoundtripTest.java
  • isthmus/src/test/java/io/substrait/isthmus/DynamicParameterTest.java

Tests/checks run and results.

  • Local: ./gradlew build --rerun-tasks with JDK 17.0.19-tem -> PASS
  • Local lint/format gate included in build (spotlessCheck) -> PASS
  • Local static analysis included in build (pmdMain/pmdTest) -> PASS
  • GitHub checks (gh pr checks 752) -> all reported checks PASS

Fixes committed/pushed, if any.

  • None. No completion blocker found in this diff.

Final CI status.

  • All current reported PR checks are successful.
  • Note: gh pr checks --required reports no required checks configured for this branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nielspardon nielspardon Awaiting requested review from nielspardon

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@bvolpato @nielspardon